Revise core.internal.array.equality #3142

kinke · 2020-06-22T19:16:53Z

Use memcmp for more types (incl. pointers and some static arrays), and don't require both element types to be equivalent (e.g., use it for comparing wchar[] and ushort[] too).
Simplify the non-memcmp branch - the previous code seemed to try to do what the compiler already does.
The previous code for structs without custom opEquals basically enforced -preview=fieldwise via .tupleof comparison. This breaking change with 2.078 almost certainly wasn't intended (and not mentioned in the changelog) and led to undesirable inconsistent behavior.
This reverts that change in semantics.
Avoid superfluously nested templates and convert the remaining at() helper to a module-level template to reduce the number of redundant instantiations.

dlang-bot · 2020-06-22T19:16:55Z

Thanks for your pull request and interest in making D better, @kinke! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
I have provided a detailed rationale explaining my changes
New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + druntime#3142"

kinke · 2020-06-23T15:58:52Z

Azure CI issue for the OMF job:

D:/a/1/dmd/tools/dmd2/windows/bin/dmd.exe -m32  -g -lowmem -i -Itools -version=NoMain -odtest_results -oftest_results/d_do_test.exe tools/d_do_test.d

OPTLINK (R) for Win32  Release 8.00.17
Copyright (C) Digital Mars 1989-2013  All rights reserved.
http://www.digitalmars.com/ctg/optlink.html
test_results\d_do_test.obj(d_do_test) 
 Error 42: Symbol Undefined __D4core8internal5array8equality__T8__equalsTxkTxkZQqFNaNbNiNfAxkQdZb
Error: linker exited with status 1

AFAICT, that's the DMD host compiler failing to generate a linkable d_do_test tool. A druntime change shouldn't affect this at all.

The unittest failure in https://github.com/CyberShadow/ae/blob/ae2afb17cae70c8df739fb0dccd00b6eef212015/utils/regex.d#L345-L346 is somewhat worrying. I'll try to reproduce that later with LDC.

kinke · 2020-06-23T20:28:36Z

Those 2 lines in the CyberShadow/ae repo nicely show the current problem with diverging behavior for __equals (doing a field-wise comparison of a struct's fields (if the struct has no custom opEquals), i.e., basically enforcing -preview=fieldwise). This wrongly passes with master:

Transformation expected = { type : Transformation.Type.replace, replace : { search : "from", replacement : "to", flags : "" } };
auto actual = splitRETransformation("s/from/to/");

assert(actual[0] != expected);    // single element NOT equal (compiled without -preview=fieldwise)

Transformation[] expectedSlice = [expected];
assert(actual == expectedSlice);  // equal (lowered to __equals); unfortunately, this is tested

Transformation[1] expectedSarray = [expected];
assert(actual != expectedSarray); // NOT equal (not lowered)

rainers · 2020-06-24T16:44:12Z

AFAICT, that's the DMD host compiler failing to generate a linkable d_do_test tool. A druntime change shouldn't affect this at all.

That's probably the same issue as this one: #3141 (comment)

Previously, the tools were compiled by the DMD host compiler, but linked with freshly compiled druntime/Phobos, leading to inevitable issues popping up in dlang/druntime#3141 and dlang/druntime#3142.

The previous test really depended on `-preview=fieldwise`. See dlang/druntime#3142 (comment).

kinke · 2020-06-27T17:22:33Z

The ae project has been adapted (no tagged release available yet though). See the linked PR above for more context wrt. that issue, boiling down to:

union Transformation
{
    string search;
}

void main()
{
    static immutable from = "from", from2 = "from2";
    Transformation[] a = [{ search : from }];
    Transformation[] b = [{ search : from2[0..4] }];
    assert(a == b);
}

failing the assertion now with this PR (using memcmp (is) without -preview=fieldwise), as it used to before v2.078, presumably when __equals (with diverging semantics) was introduced (I haven't found any mention of this changed behavior in the changelog).

schveiguy · 2020-07-08T17:38:32Z

oops, didn't know there was already something for this instead of #3151.

Is there anything in this that still needs merging? I think it would need to at least be rebased now that the other PR is merged.

kinke · 2020-07-08T17:47:27Z

Yeah, a rebase is now necessary, but it's still blocked by the missing new tag for @CyberShadow's ae repo.

n8sh · 2020-07-08T20:14:15Z

src/core/internal/array/equality.d

+    // Use memcmp if applicable to both element types and the size matches.
+    static if (T1.sizeof == T2.sizeof && useMemcmp!T1 && useMemcmp!T2)


Need to also check __traits(isUnsigned, T1) == __traits(isUnsigned, T2) or else:

ubyte[] a = [cast(ubyte) 0xff]; byte[] b = [cast(byte) 0xff]; assert(a != b); // Currently passes but will fail after this PR.

Right, thx. That only affects integers < 32 bit though, due to int promotion.

n8sh · 2020-07-08T20:16:48Z

src/core/internal/array/equality.d

+    import core.internal.traits : Unqual;
+    enum isVoid = is(Unqual!T == void);


From the perspective of the compiler frontend there anything to recommend this over is(immutable T == immutable void)?

What about shared?

It works since immutable(shared T) == immutable T.

From the perspective of the compiler frontend there anything to recommend this over is(immutable T == immutable void)?

Absolutely not, thx for the suggestion.

* Use memcmp for more types (incl. pointers and some static arrays), and don't require both element types to be equivalent (e.g., use it for comparing wchar[] and ushort[] too). * Simplify the non-memcmp branch - the previous code seemed to try to do what the compiler already does. * The previous code for structs without custom opEquals basically enforced -preview=fieldwise via .tupleof comparison. This breaking change with 2.078 almost certainly wasn't intended (and not mentioned in the changelog) and led to undesirable inconsistent behavior, see dlang#3142 (comment). This reverts that change in semantics. * Avoid superfluously nested templates and convert the remaining `at()` helper to a module-level template to reduce the number of redundant instantiations.

n8sh · 2020-07-08T23:45:58Z

Those 2 lines in the CyberShadow/ae repo nicely show the current problem with diverging behavior for __equals (doing a field-wise comparison of a struct's fields (if the struct has no custom opEquals), i.e., basically enforcing -preview=fieldwise).

I think everyone would agree that a fix for something like this should appear in the changelog.

* Use memcmp for more types (incl. pointers and some static arrays), and don't require both element types to be equivalent (e.g., use it for comparing wchar[] and ushort[] too). * Simplify the non-memcmp branch - the previous code seemed to try to do what the compiler already does. * The previous code for structs without custom opEquals basically enforced -preview=fieldwise via .tupleof comparison. This breaking change with 2.078 almost certainly wasn't intended (and not mentioned in the changelog) and led to undesirable inconsistent behavior, see dlang#3142 (comment). This reverts that change in semantics. * Avoid superfluously nested templates and convert the remaining `at()` helper to a module-level template to reduce the number of redundant instantiations.

kinke · 2020-07-24T17:50:45Z

The ae repo is no blocker anymore (new tag available).

kinke · 2020-07-24T20:05:49Z

Thx @n8sh. I'm sure you'll update and re-evaluate #3152 (sorry, I haven't found the time to perform the promised tests yet).

JohanEngelen · 2020-07-30T11:06:17Z

Because it removed all the casting, this PR fixed this bug: https://issues.dlang.org/show_bug.cgi?id=21094.

* Use memcmp for more types (incl. pointers and some static arrays), and don't require both element types to be equivalent (e.g., use it for comparing wchar[] and ushort[] too). * Simplify the non-memcmp branch - the previous code seemed to try to do what the compiler already does. * The previous code for structs without custom opEquals basically enforced -preview=fieldwise via .tupleof comparison. This breaking change with 2.078 almost certainly wasn't intended (and not mentioned in the changelog) and led to undesirable inconsistent behavior, see dlang#3142 (comment). This reverts that change in semantics. * Avoid superfluously nested templates and convert the remaining `at()` helper to a module-level template to reduce the number of redundant instantiations. See dlang#3142

dlang/druntime#3142

Previously, the tools were compiled by the DMD host compiler, but linked with freshly compiled druntime/Phobos, leading to inevitable issues popping up in dlang/druntime#3141 and dlang/druntime#3142.

kinke requested review from MartinNowak, andralex and schveiguy as code owners June 22, 2020 19:16

kinke force-pushed the arrayequals branch 3 times, most recently from 4f2c1ce to df01095 Compare June 23, 2020 08:53

kinke mentioned this pull request Jun 25, 2020

Azure CI OMF job: Link testsuite tools with correct phobos.lib dlang/dmd#11322

Merged

kinke force-pushed the arrayequals branch from df01095 to 7042349 Compare June 27, 2020 11:55

kinke mentioned this pull request Jun 27, 2020

utils.regex: Fix unittest depending on unsound array equality check CyberShadow/ae#44

Closed

CyberShadow pushed a commit to CyberShadow/ae that referenced this pull request Jun 27, 2020

ae.utils.regex: Fix unittest depending on unsound array equality check

994f4ac

The previous test really depended on `-preview=fieldwise`. See dlang/druntime#3142 (comment).

kinke mentioned this pull request Jul 8, 2020

Remove __ArrayEq which the compiler no longer uses as of https://github.com/dlang/dmd/pull/11212 #3151

Merged

n8sh reviewed Jul 8, 2020

View reviewed changes

n8sh mentioned this pull request Jul 8, 2020

Reduce template function instantiations related to array equality #3152

Merged

kinke force-pushed the arrayequals branch from 7042349 to 0d0a96a Compare July 8, 2020 23:17

kinke force-pushed the arrayequals branch from 0d0a96a to dd24691 Compare July 9, 2020 00:44

kinke requested a review from wilzbach as a code owner July 9, 2020 00:44

kinke force-pushed the arrayequals branch from dd24691 to cdc23f3 Compare July 24, 2020 17:22

n8sh approved these changes Jul 24, 2020

View reviewed changes

n8sh added the auto-merge label Jul 24, 2020

dlang-bot merged commit cc1ed93 into dlang:master Jul 24, 2020

kinke deleted the arrayequals branch July 24, 2020 20:06

JohanEngelen added a commit to weka/ldc that referenced this pull request Jul 30, 2020

Cherry-pick array comparison fix from upstream druntime master.

d2dc8d7

dlang/druntime#3142

kinke mentioned this pull request Sep 29, 2020

Get rid of obsolete __ArrayEq lowering & revise core.internal.array.equality ldc-developers/ldc#3456

Closed

		// Use memcmp if applicable to both element types and the size matches.
		static if (T1.sizeof == T2.sizeof && useMemcmp!T1 && useMemcmp!T2)

		import core.internal.traits : Unqual;
		enum isVoid = is(Unqual!T == void);

Uh oh!

Revise core.internal.array.equality #3142

Revise core.internal.array.equality #3142

Uh oh!

Conversation

kinke commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Jun 22, 2020

Bugzilla references

Testing this PR locally

Uh oh!

kinke commented Jun 23, 2020

Uh oh!

kinke commented Jun 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rainers commented Jun 24, 2020

Uh oh!

kinke commented Jun 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schveiguy commented Jul 8, 2020

Uh oh!

kinke commented Jul 8, 2020

Uh oh!

n8sh Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

kinke Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

n8sh Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

MoonlightSentinel Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

n8sh Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

kinke Jul 8, 2020

Choose a reason for hiding this comment

Uh oh!

n8sh commented Jul 8, 2020

Uh oh!

kinke commented Jul 24, 2020

Uh oh!

kinke commented Jul 24, 2020

Uh oh!

JohanEngelen commented Jul 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

kinke commented Jun 22, 2020 •

edited

Loading

kinke commented Jun 23, 2020 •

edited

Loading

kinke commented Jun 27, 2020 •

edited

Loading